-
Notifications
You must be signed in to change notification settings - Fork 35
enable html5 notifications #464
base: xenial
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm. This seems like a huge review without much ability for me to give useful feedback. But I did find a couple of things.
remove commented code line
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, that's the static analysis done.
Note that this DOES NOT enable webpush. At all. Attempting to use the Microsoft Edge Web Push demo throws the error Registration failed - push service not available
in the browser console.
Instead, this only enables the Notifications API. The notifications API allows an active tab to send a notification via JavaScript. If a tab becomes suspended due to memory pressure, it will be unable to send notifications any more. This limits the feature's usefulness for now, but could lead to better things in the future.
I tested the feature with https://www.davidwalsh.name/demo/notifications-api.php.
When a website tries to show a notification, a permission request appears.
If allowed, the single notification that the website tried to send appears.
If the website tries to send another notification, the permission request appears again. I believe this should be changed. It is not useful to be asked whether to allow or deny a push notification every time a website tries to send it... you might as well have just allowed the notification in the first place. Either the "Remember decision" checkbox should be checked by default or, probably more useful, removed and the options changed to "Always Allow" and "Always Deny"
Also interesting: Tapping on a notification opens the website attached to the notification in a new tab. I think that the browser should switch to the site's open tab instead of opening a new tab. That's the behavior of other browsers, at least. |
for the first issue (authentication asked twice): I think getting rid of the "allow permanently" checkbox is not possible, because in incognito mode it is not enabled because no permanent data about domains is saved then. The second one seems a bit trickier: We would need to change the external URL handling and not open it in another tab, if there is already an open tab with that URL. You're right other browser do it like that. |
Yes. I didn't leave the page. I just clicked the button to send a notification, said "no" to the prompt, then clicked the button to send a notification again.
That seems fine. Firefox silently changes this behavior in Private Mode, the "Allow" button changes from enabling "Allowed" to "Allowed temporarily".
True. I think it's required, though. Take Firefox on https://www.davidwalsh.name/demo/notifications-api.php as an example: if you close the tab, the notification is removed from Gnome! The notifications aren't meant to outlive a tab, and they aren't meant to have any other context than that. |
ah ok, that's true the "no" was not saved, but the "yes" is. For the last issue (focus tab instead of opening the link in the new tab): For morph as the default URL handler we could generally change the logic like that:
Would it be a problem, if all URL links (e.g. links clicked in apps) are handled that way by morph-browser ? |
that would be different to an URL passed on the desktop via |
…ad of opening a new one
ok with the latest commit, morph switches to the (already open) tab with the url from the notification |
No description provided.